Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Adding ability for the Maven object to create the settings file #43

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

shawn-hurley
Copy link
Contributor

We are also adding ability to set the local repository to the M2Dir.

repository/maven.go Outdated Show resolved Hide resolved
repository/maven.go Outdated Show resolved Hide resolved
@shawn-hurley shawn-hurley changed the title :Bug: Adding ability for the Maven object to create the settings file 🐛 Adding ability for the Maven object to create the settings file Oct 30, 2023
@shawn-hurley shawn-hurley requested review from mansam and jortel November 6, 2023 20:34
@shawn-hurley
Copy link
Contributor Author

@jortel @mansam This is ready for review, please let me know if there is something that you need changed

if err != nil {
err = liberr.Wrap(
err,
"path",
path)
}
_ = f.Close()
addon.Activity("[FILE] Created %s.", path)
addon.Activity("[FILE] Created %s. -- file: %s", path, settings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot include the settings file in the task activity log because it contains credentials.

return
}

//
func (r *Maven) injectCacheDir(settings mxj.Map) (mxj.Map, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mxj.Map isA map which are passed/modified by reference. I don't see the point in returned it.
All of the core projects use named return values by convention. Please use named return values for consistency.

func (r *Maven) injectProxy(id *api.Identity) (s string, err error) {
s = id.Settings
m, err := mxj.NewMapXml([]byte(id.Settings))
func (r *Maven) injectProxy(id *api.Identity) (mxj.Map, error) {
Copy link
Contributor

@jortel jortel Nov 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the core projects use named return values by convention. Why did you change this?
Also, my previous comment requested the injectProxy() take a mxj.Map to be consistent with injectCacheDir() and because both the m2 dir and proxy settings should be set on either the emptySettings or the setting in the Identity. Right?

@jortel
Copy link
Contributor

jortel commented Nov 7, 2023

Last I checked, ubi9 contains Go 1.18 so we have been development with it to ensure compatibility. I assume the widespread reformatting was because you used gomft 1.19+ which has, imho, become rather draconian. If so, that's fine, all of the files will get reformatted when the project updates Go. If not, please revert the unrelated changes.

@shawn-hurley shawn-hurley force-pushed the bugfix/issue-46 branch 2 times, most recently from 368ab6b to 29afdb4 Compare November 8, 2023 19:24
…local repo set

Signed-off-by: Shawn Hurley <shawn@hurley.page>
Copy link
Contributor

@jortel jortel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks @shawn-hurley !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants